fix: handle compound commands in terminal integration #7432
Conversation
- Add compound command detection to track multi-process commands - Implement process completion tracking for operators like &&, ||, ;, | - Wait for all processes in compound commands before reporting to LLM - Add comprehensive tests for compound command scenarios Fixes #7430
- Enhanced compound command detection logic - Added more comprehensive test coverage - Fixed edge cases in process completion tracking
dosubot
bot
added
size:XXL
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| const compoundOperators = ["&&", "||", ";", "|", "&"] | ||
|
|
||
| // Check if command contains any compound operators | ||
| this.isCompoundCommand = compoundOperators.some((op) => command.includes(op)) |
There was a problem hiding this comment.
The compound command detection using simple string matching could incorrectly identify operators within quoted strings. For example, echo "test && test" would be incorrectly flagged as a compound command. Could we consider using a more robust parsing approach that respects shell quoting rules?
| const semiMatches = command.match(/;/g) | ||
| const pipeMatches = command.match(/\|(?!\|)/g) // Match single | but not || | ||
| // Match single & but not &&, and not preceded by & | ||
| const bgMatches = command.match(/(?<!&)&(?!&)/g) |
There was a problem hiding this comment.
Is the negative lookbehind (?<!&)&(?!&) supported in all JavaScript environments we target? Some older environments might not support lookbehind assertions. Would a more compatible regex pattern work here?
| ) | ||
| this.finalizeCompoundCommand() | ||
| } | ||
| }, 10000) // 10 second timeout for compound commands |
There was a problem hiding this comment.
The 10-second timeout is hardcoded here. Could we extract this as a named constant like COMPOUND_COMMAND_TIMEOUT_MS or make it configurable? This would make it easier to adjust if needed and improve code readability.
| * Gets the combined output from all compound processes | ||
| * @returns The combined output string | ||
| */ | ||
| public getCompoundProcessOutputs(): string { |
There was a problem hiding this comment.
This method only returns metadata about the commands and exit codes, but doesn't capture the actual command output. Since the original issue was about missing output from subsequent processes, should we also be collecting and returning the actual output from each process?
| public finalizeCompoundCommand(): void { | ||
| // Clear the timeout if it exists | ||
| if (this.compoundCommandWaitTimeout) { | ||
| clearTimeout(this.compoundCommandWaitTimeout) |
There was a problem hiding this comment.
If an error occurs during compound command execution, we might not reach this cleanup code. Should we wrap the compound command execution in a try-finally block to ensure the timeout is always cleared?
| { | ||
| terminalId: terminal.id, | ||
| command: e.execution?.commandLine?.value, | ||
| exitCode: e.exitCode, |
There was a problem hiding this comment.
The extensive console.info logging here and throughout the compound command handling might be too verbose for production. Could we use debug-level logging or make the verbosity configurable?
hannesrudolph
added
the
Issue/PR - Triage
|
The issue needs scoping |
Summary
This PR fixes the issue where compound commands (using operators like
&&,||,;,|) were not properly handled by the terminal integration. Previously, the TerminalRegistry would incorrectly report completion after the first process in a compound command finished, missing output from subsequent processes.Problem
As described by @pwilkin in #7430:
cd dir && command_with_outputSolution
Implemented multi-process tracking for compound commands:
Changes
Testing
&&,||,;,|,&)Fixes #7430
Important
Fixes compound command handling in terminal integration by tracking all processes in a command chain before completion.
TerminalRegistry, ensuring all processes in a command chain are tracked and completed before reporting to LLM.&&,||,;,|,&.BaseTerminal: AddsdetectCompoundCommand(),addCompoundProcessCompletion(),allCompoundProcessesComplete(),getCompoundProcessOutputs(), andfinalizeCompoundCommand()for compound command tracking.TerminalandExecaTerminal: Integrate compound command detection inrunCommand().TerminalRegistry: Updates shell execution handlers to manage compound command processes.RooTerminalinterface with compound command properties.CompoundCommand.spec.ts,CompoundCommandSimple.spec.ts, andTerminalRegistryCompound.spec.tsfor testing compound command scenarios.This description was created by
for f68a9ab. You can customize this summary. It will automatically update as commits are pushed.